fix(storage): cleanup tail in dyn types#3840
Conversation
0xrusowsky
force-pushed
the
rus/clear-dyn-writes
branch
from
0b5e41b to
33254c0
Compare
📊 Tempo Precompiles CoverageprecompilesCoverage: 5639/7749 lines (72.77%) File details
contractsCoverage: 1/253 lines (0.40%) File details
Total: 5640/8002 lines (70.48%) |
0xrusowsky
requested review from
0xKitsune,
fgimenez,
klkvr,
legion2002 and
mattsse
as code owners
| // Handlers always use `FULL` ctx: | ||
| // `T::Handler::write(v)` → `self.as_slot().write(v)` → `Slot::<T>::new(s, a).write(v)`. | ||
| // Since the slot we push to is guaranteed empty, we build the `Slot<T>` directly to | ||
| // thread `INIT` into `T::store` and skip its tail-cleanup SLOAD for dynamic types. | ||
| let elem_slot = self.data_slot() + U256::from(length * T::SLOTS); | ||
| Slot::<T>::new_with_ctx(elem_slot, LayoutCtx::INIT, self.address).write(value)?; |
There was a problem hiding this comment.
NOTE: i believe this workaround is safe because of the reasons explained in the cmnt
in order to use Self::compute_handler with LayoutCtx::INIT we'd have to change all non-primitive handlers to use and hold ctx (which currently ignore)
There was a problem hiding this comment.
can we just call T::store directly here?
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Four worker passes (w1×3, w2×2) returned NO_FINDING for any T2-active vulnerability. The PR is well-scoped and the new T5-gated cleanup branches plus LayoutCtx::INIT are deterministic, bounded, and consistent with the typed-storage invariants.
Head drift note: workers audited 0b5e41b40efb; this review targets bcc60e304e3fcdad7818f75618fe71019cde6104. The drift moved gating from T4 to T5 and fixed the previously-broken test compile (ONE → U256::ONE in structs.rs:309), so that finding has been dropped as stale.
🛡️ [DEFENSE-IN-DEPTH] [T; N] Storable impl not migrated to is_full()
File: crates/precompiles-macros/src/storable_primitives.rs:378,387,399 (not in this PR's diff — body-only)
Sister types (Vec, String, Bytes, struct macro) were uniformly updated to debug_assert!(ctx.is_full(), …), but gen_array_impl's generated handle/load/store still use debug_assert_eq!(ctx, LayoutCtx::FULL, …). Today this is dormant — no production Vec<[T; N]> exists and the struct macro forces FULL on non-dynamic fields. But if anyone adds Vec<[u8; 32]> (or any [T; N] with T::BYTES * N > 16), Vec::push will thread INIT into [T; N]::store and the equality assert will fire in debug/test builds (release silently works because store_impl doesn't consult ctx).
Recommended Fix: Replace the three debug_assert_eq!(ctx, LayoutCtx::FULL, …) calls with debug_assert!(ctx.is_full(), …) for symmetry.
See inline comments for the other two items.
Reviewer Callouts
-
⚡ Gas-cost change for shrinking dynamic types under T5 —
Vec::storeandstore_bytes_likeadd up toprev - newextra SSTOREs (orT::deletecalls for unpacked) on T5. Gas-metered and not griefable, but per-operation costs of shrinking change post-T5. Worth confirming downstream precompile gas budgets (e.g.,account_keychainrevoke flows,stablecoin_dexbook mutations, validator config rewrites with long string fields) accommodate the additional SSTOREs. -
⚡
LayoutCtx::INIT"virgin by construction" forVec::pushis fork-history sensitive — Holds for anyVecwhose entire lifetime is on T5 (every shrinking path zeros freed elements). Does NOT hold for aVecwhose history includes a pre-T5 shrink. Concretely,write(vec![long_a, long_b, long_c])thenwrite(vec![x, y])under T2/T3/T4 leaveslong_c's chunks live; a later T5push("z")to index 2 writes the base slot withINITbut the legacy chunks atkeccak256(data_start + 2) + ipersist. Impact is purely storage bloat — every read path is length-bounded andkeccak256precludes collisions with live storage. -
⚡ Pre-T5 → T5 activation hygiene — Combined with the bytes_like inline finding, a human reviewer should confirm the T5 activation plan accounts for legacy dynamic tails that may already exist when T5 activates. Cleanup only runs on subsequent writes/deletes; pre-existing ghost data in long validator config strings, TIP-20 metadata, etc. is never proactively scrubbed.
-
⚡
Mapping::IS_DYNAMICdefaults tofalse(storage/types/mod.rs:182) —Mappingis keccak-addressed but does not implementStorable(onlyStorableType), so the derive splits mapping fields out before callingstore/load/delete. Harmless today, but if a future change adds aStorableimpl toMapping, the propagation logic ingen_store_implwould need updating.
Collects bug fixes and infrastructure changes gated behind T5: 1. Fix fixed-size array packing in precompile storage codegen (#3811) 2. Clean up stale tail slots in dynamic storage types (#3840) 3. Deploy ERC-2470 and NanoUniversalDeployer at T5 boundary (#3742) Standalone feature TIPs (1026, 1030, 1033, 1035, 1047, 1056) are excluded — they already have their own specs. Amp-Thread-ID: https://ampcode.com/threads/T-019dfe69-9662-77ff-9ff0-390655ec07ff
Collects bug fixes gated behind T5: 1. Fix fixed-size array packing in precompile storage codegen (#3811) 2. Clean up stale tail slots in dynamic storage types (#3840) Standalone feature TIPs (1026, 1030, 1033, 1035, 1047, 1056) are excluded — they already have their own specs. Amp-Thread-ID: https://ampcode.com/threads/T-019dfe69-9662-77ff-9ff0-390655ec07ff
| // Vec elements can't be split across slots. | ||
| let from_slots = calc_packed_slot_count(from, T::BYTES); | ||
| let to_slots = calc_packed_slot_count(to, T::BYTES); | ||
| for slot_idx in from_slots..to_slots { | ||
| storage.store(data_start + U256::from(slot_idx), U256::ZERO)?; | ||
| } |
There was a problem hiding this comment.
looks like if from_slots == to_slots this won't clear anything?
There was a problem hiding this comment.
yes, and i think that's correct (i should probably document it better).
my reasoning is as follows:
- delete always uses
clear_elements(.., from=0, to=length), which means it always deletes smth - tail cleanup doesn't delete the same element cause it is overriden by the later SSTORE, so it would be redundant
…t_array_impl (tempoxyz#3860) Follow-up to tempoxyz#3840 — the five `debug_assert_eq!(ctx, LayoutCtx::FULL)` checks in `gen_array_impl` and `gen_struct_array_impl` (`storable_primitives.rs`) were not updated when the `LayoutCtx::INIT` sentinel was introduced. This switches them to `debug_assert!(ctx.is_full())` so passing `INIT` no longer trips the assertion in debug builds. - 3 assertions in `gen_array_impl` (handle / load / store) - 2 assertions in `gen_struct_array_impl` (load / store) All 89 storage tests pass. Co-authored-by: Centaur AI <ai@centaur.local>
4 participants
Motivation
Overwriting a dynamic storable (
Vec<T>,String,Bytes) with a shorter value left stale tail slots populated, so subsequent reads observed garbage past the new length.Solution
Ensure that "shrinking" writes on dyn types clear their stale tails.
Additionally, introduces a new sentinel
LayoutCtx::INITfor hot paths that know the destination is virgin (Vec::push), letting them skip the extra SLOAD + cleanup.